Automated Test: feature-recovery-keys-implementation #318
+256
−31
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
closes #38445 Signed-off-by: rtufisi <rtufisi@phasetwo.io>
📝 WalkthroughWalkthroughThe changes introduce Recovery Codes credential support across multiple authentication components, consolidating credential retrieval and creation logic through new utility methods while extending backwards-compatibility storage to handle recovery code storage and validation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java`:
- Around line 17-21: The code currently calls credentialModelOpt.get() without
checking the Optional returned by RecoveryAuthnCodesUtils.getCredential(user),
which can throw; update the logic in RecoveryAuthnCodeInputLoginBean to guard
the Optional (using isPresent()/ifPresent(), orElseThrow() with a clear message,
or provide a fallback) before calling
RecoveryAuthnCodesCredentialModel.createFromCredentialModel and accessing
getNextRecoveryAuthnCode().get().getNumber(), and ensure codeNumber is set or a
clear exception is raised when no credential exists.
In
`@testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java`:
- Around line 326-341: In BackwardsCompatibilityUserStorage, replace the raw
List used for deserializing recovery codes (generatedKeys) with a typed
List<String> when calling JsonSerialization.readValue to restore type safety,
handle potential null elements by using Objects.equals(key,
input.getChallengeResponse()) or by comparing
input.getChallengeResponse().equals(key) to avoid NPEs, and fix the extra space
before the else; also add import java.util.Objects if you use Objects.equals.
- Around line 236-246: The deserialization uses a raw List.class and swallows
exceptions: change the JsonSerialization.readValue call to use a typed
TypeReference<List<String>> so
RecoveryAuthnCodesCredentialModel.createFromValues receives a List<String>, and
update the catch(IOException e) in the block around
RecoveryAuthnCodesCredentialModel.createFromValues to not silently ignore
failures—either rethrow a runtime exception or propagate the IOException (or at
minimum log the error with e as parameter) so callers know deserialization
failed; also fix the double-space typo in the log message ("deserialize
credential" -> "deserialize credential"). Reference:
RecoveryAuthnCodesCredentialModel.createFromValues, JsonSerialization.readValue,
and the catch(IOException e) logging line.
Comment on lines
+17
to
21
| Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user); | ||
|
|
||
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModel); | ||
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get()); | ||
|
|
||
| this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode().get().getNumber(); |
There was a problem hiding this comment.
Guard the Optional to avoid runtime failures.
If getCredential(user) returns empty, credentialModelOpt.get() will throw and break the login page. Add an explicit guard (or fail fast with a clear message) before accessing the credential.
🛠️ Proposed fix
- Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user);
-
- RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get());
-
- this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode().get().getNumber();
+ Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user);
+ RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = credentialModelOpt
+ .map(RecoveryAuthnCodesCredentialModel::createFromCredentialModel)
+ .orElseThrow(() -> new IllegalStateException("Recovery codes credential missing for user " + user.getUsername()));
+
+ this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode()
+ .orElseThrow(() -> new IllegalStateException("No recovery codes available for user " + user.getUsername()))
+ .getNumber();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user); | |
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModel); | |
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = RecoveryAuthnCodesCredentialModel.createFromCredentialModel(credentialModelOpt.get()); | |
| this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode().get().getNumber(); | |
| Optional<CredentialModel> credentialModelOpt = RecoveryAuthnCodesUtils.getCredential(user); | |
| RecoveryAuthnCodesCredentialModel recoveryCodeCredentialModel = credentialModelOpt | |
| .map(RecoveryAuthnCodesCredentialModel::createFromCredentialModel) | |
| .orElseThrow(() -> new IllegalStateException("Recovery codes credential missing for user " + user.getUsername())); | |
| this.codeNumber = recoveryCodeCredentialModel.getNextRecoveryAuthnCode() | |
| .orElseThrow(() -> new IllegalStateException("No recovery codes available for user " + user.getUsername())) | |
| .getNumber(); |
🤖 Prompt for AI Agents
In
`@services/src/main/java/org/keycloak/forms/login/freemarker/model/RecoveryAuthnCodeInputLoginBean.java`
around lines 17 - 21, The code currently calls credentialModelOpt.get() without
checking the Optional returned by RecoveryAuthnCodesUtils.getCredential(user),
which can throw; update the logic in RecoveryAuthnCodeInputLoginBean to guard
the Optional (using isPresent()/ifPresent(), orElseThrow() with a clear message,
or provide a fallback) before calling
RecoveryAuthnCodesCredentialModel.createFromCredentialModel and accessing
getNextRecoveryAuthnCode().get().getNumber(), and ensure codeNumber is set or a
clear exception is raised when no credential exists.
Comment on lines
+236
to
+246
| try { | ||
| model = RecoveryAuthnCodesCredentialModel.createFromValues( | ||
| JsonSerialization.readValue(myUser.recoveryCodes.getCredentialData(), List.class), | ||
| myUser.recoveryCodes.getCreatedDate(), | ||
| myUser.recoveryCodes.getUserLabel() | ||
| ); | ||
| credentialModels.add(model); | ||
| } catch (IOException e) { | ||
| log.error("Could not deserialize credential of type: recovery-codes"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Raw List type and silently swallowed exception.
- Line 238 uses raw
List.class, causing an unchecked conversion when passed tocreateFromValues(List<String>, ...). This loses type safety. - The caught
IOExceptiononly logs and continues, meaning recovery codes will silently be missing from the returned stream. Callers won't know deserialization failed. - Line 244 has a double space typo:
"deserialize credential".
Suggested improvements
if (myUser.recoveryCodes != null) {
try {
+ `@SuppressWarnings`("unchecked")
+ List<String> codes = JsonSerialization.readValue(myUser.recoveryCodes.getCredentialData(), List.class);
model = RecoveryAuthnCodesCredentialModel.createFromValues(
- JsonSerialization.readValue(myUser.recoveryCodes.getCredentialData(), List.class),
+ codes,
myUser.recoveryCodes.getCreatedDate(),
myUser.recoveryCodes.getUserLabel()
);
credentialModels.add(model);
} catch (IOException e) {
- log.error("Could not deserialize credential of type: recovery-codes");
+ log.error("Could not deserialize credential of type: recovery-codes", e);
+ throw new RuntimeException("Failed to deserialize recovery codes credential", e);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| model = RecoveryAuthnCodesCredentialModel.createFromValues( | |
| JsonSerialization.readValue(myUser.recoveryCodes.getCredentialData(), List.class), | |
| myUser.recoveryCodes.getCreatedDate(), | |
| myUser.recoveryCodes.getUserLabel() | |
| ); | |
| credentialModels.add(model); | |
| } catch (IOException e) { | |
| log.error("Could not deserialize credential of type: recovery-codes"); | |
| } | |
| } | |
| try { | |
| `@SuppressWarnings`("unchecked") | |
| List<String> codes = JsonSerialization.readValue(myUser.recoveryCodes.getCredentialData(), List.class); | |
| model = RecoveryAuthnCodesCredentialModel.createFromValues( | |
| codes, | |
| myUser.recoveryCodes.getCreatedDate(), | |
| myUser.recoveryCodes.getUserLabel() | |
| ); | |
| credentialModels.add(model); | |
| } catch (IOException e) { | |
| log.error("Could not deserialize credential of type: recovery-codes", e); | |
| throw new RuntimeException("Failed to deserialize recovery codes credential", e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java`
around lines 236 - 246, The deserialization uses a raw List.class and swallows
exceptions: change the JsonSerialization.readValue call to use a typed
TypeReference<List<String>> so
RecoveryAuthnCodesCredentialModel.createFromValues receives a List<String>, and
update the catch(IOException e) in the block around
RecoveryAuthnCodesCredentialModel.createFromValues to not silently ignore
failures—either rethrow a runtime exception or propagate the IOException (or at
minimum log the error with e as parameter) so callers know deserialization
failed; also fix the double-space typo in the log message ("deserialize
credential" -> "deserialize credential"). Reference:
RecoveryAuthnCodesCredentialModel.createFromValues, JsonSerialization.readValue,
and the catch(IOException e) logging line.
Comment on lines
+326
to
+341
| } else if (input.getType().equals(RecoveryAuthnCodesCredentialModel.TYPE)) { | ||
| CredentialModel storedRecoveryKeys = myUser.recoveryCodes; | ||
| if (storedRecoveryKeys == null) { | ||
| log.warnf("Not found credential for the user %s", user.getUsername()); | ||
| return false; | ||
| } | ||
| List generatedKeys; | ||
| try { | ||
| generatedKeys = JsonSerialization.readValue(storedRecoveryKeys.getCredentialData(), List.class); | ||
| } catch (IOException e) { | ||
| log.warnf("Cannot deserialize recovery keys credential for the user %s", user.getUsername()); | ||
| return false; | ||
| } | ||
|
|
||
| return generatedKeys.stream().anyMatch(key -> key.equals(input.getChallengeResponse())); | ||
| } else { |
There was a problem hiding this comment.
Raw type usage and potential NPE in recovery code validation.
- Line 332/334: Raw
Listtype loses type safety; same issue as ingetCredentials. - Line 340: If the deserialized list contains
nullelements (malformed data),key.equals(...)will throwNullPointerException. Consider usingObjects.equals()or reversing the comparison. - Line 341: Minor formatting issue - double space before
else.
Suggested fix
- List generatedKeys;
+ `@SuppressWarnings`("unchecked")
+ List<String> generatedKeys;
try {
generatedKeys = JsonSerialization.readValue(storedRecoveryKeys.getCredentialData(), List.class);
} catch (IOException e) {
log.warnf("Cannot deserialize recovery keys credential for the user %s", user.getUsername());
return false;
}
- return generatedKeys.stream().anyMatch(key -> key.equals(input.getChallengeResponse()));
- } else {
+ String challengeResponse = input.getChallengeResponse();
+ return generatedKeys.stream().anyMatch(key -> Objects.equals(key, challengeResponse));
+ } else {Note: Add import java.util.Objects; at the top of the file.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else if (input.getType().equals(RecoveryAuthnCodesCredentialModel.TYPE)) { | |
| CredentialModel storedRecoveryKeys = myUser.recoveryCodes; | |
| if (storedRecoveryKeys == null) { | |
| log.warnf("Not found credential for the user %s", user.getUsername()); | |
| return false; | |
| } | |
| List generatedKeys; | |
| try { | |
| generatedKeys = JsonSerialization.readValue(storedRecoveryKeys.getCredentialData(), List.class); | |
| } catch (IOException e) { | |
| log.warnf("Cannot deserialize recovery keys credential for the user %s", user.getUsername()); | |
| return false; | |
| } | |
| return generatedKeys.stream().anyMatch(key -> key.equals(input.getChallengeResponse())); | |
| } else { | |
| } else if (input.getType().equals(RecoveryAuthnCodesCredentialModel.TYPE)) { | |
| CredentialModel storedRecoveryKeys = myUser.recoveryCodes; | |
| if (storedRecoveryKeys == null) { | |
| log.warnf("Not found credential for the user %s", user.getUsername()); | |
| return false; | |
| } | |
| `@SuppressWarnings`("unchecked") | |
| List<String> generatedKeys; | |
| try { | |
| generatedKeys = JsonSerialization.readValue(storedRecoveryKeys.getCredentialData(), List.class); | |
| } catch (IOException e) { | |
| log.warnf("Cannot deserialize recovery keys credential for the user %s", user.getUsername()); | |
| return false; | |
| } | |
| String challengeResponse = input.getChallengeResponse(); | |
| return generatedKeys.stream().anyMatch(key -> Objects.equals(key, challengeResponse)); | |
| } else { |
🤖 Prompt for AI Agents
In
`@testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/federation/BackwardsCompatibilityUserStorage.java`
around lines 326 - 341, In BackwardsCompatibilityUserStorage, replace the raw
List used for deserializing recovery codes (generatedKeys) with a typed
List<String> when calling JsonSerialization.readValue to restore type safety,
handle potential null elements by using Objects.equals(key,
input.getChallengeResponse()) or by comparing
input.getChallengeResponse().equals(key) to avoid NPEs, and fix the extra space
before the else; also add import java.util.Objects if you use Objects.equals.
Sign in
to join this conversation on GitHub.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Tests